Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine AspectJ plugin build phase config #3284

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kriegaex
Copy link
Contributor

@kriegaex kriegaex commented Dec 28, 2023

This is a follow-up on #3282, where a reviewer's misunderstanding (running test builds with wrong Maven phases) led to parts of the PR getting falsely erased. This PR not only reinstates those erased parts, but also enables developers who do not know the build configuration well enough to know that they should run mvn process-classes rather than mvn compile to now use the latter. This new convenience feature is not a fix or change of the previous PR, but changes a build phase which has been there long before the PR.

@mp911de, @odrotbohm

@kriegaex kriegaex mentioned this pull request Dec 28, 2023
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 28, 2023
@kriegaex kriegaex changed the title Improve aspectj build 2 Improve AspectJ build (2) Dec 28, 2023
Document the pros and cons in the POM with this comment:

Attention: aspectj-maven-plugin MUST be declared AFTER
maven-compiler-plugin, if they are both supposed to run in the same
default phases 'compile' and 'test-compile', but execution order is to
be guaranteed. Then, you have the convenience of being able to run e.g.
'mvn [clean] compile' instead of 'mvn [clean] process-classes'. For a
slightly less convenient, but less refactoring-error-prone solution,
see the commented-out phases below.
@kriegaex
Copy link
Contributor Author

kriegaex commented Dec 29, 2023

@mp911de, like I said over there in the other PR, after your ANTLR optimisation and removal of Replacer plugin, I dropped the first commit because it is now obsolete, leaving only the AspectJ Maven phase change to default. I still think, this would help more than it would hurt. It is documented well enough to avoid anyone messing up in the future. And if they would mess up, they would notice immediately, because the build would break.

@mp911de
Copy link
Member

mp911de commented Dec 29, 2023

Alright, we're on the same page now. Care to remove the commented-out XML bits so I can merge the PR without polishing it? Happy to keep the explanatory comment though.

@mp911de mp911de changed the title Improve AspectJ build (2) Refine AspectJ plugin build phase config Dec 29, 2023
@mp911de mp911de added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 29, 2023
@kriegaex
Copy link
Contributor Author

Actually, I left them there on purpose, and the comment even points to them. I like things to be explicit rather than expecting other developers to possess sacred knowledge. But if you insist, of course I can remove them, as soon as I am near my workstation again. 🙂

@kriegaex
Copy link
Contributor Author

OK, done, even though I still think that keeping the phases and the hint as a help for the future would be better. You can either merge the whole PR with both commits or cherry-pick just the first one, if you change your mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants